fix(webhooks): bracket and structurally validate DockerLocalhostRewrite rewrite_to - #996
Conversation
There was a problem hiding this comment.
Structural validation of rewrite_to at construction. Right call — the guard proves the value cannot restructure the netloc that becomes the signed @authority, which is the correct invariant for the one non-signing layer that synthesizes that authority. Real bug: bare ::1 against a ported URL produced https://::1:9000/hook, and _canon_authority's rsplit(":", 1) mis-splits that into a wrong signature rather than a failed request.
Things I checked
__post_init__on afrozen=Truedataclass mutates viaobject.__setattr__— correct for frozen dataclasses. Happy path unchanged: defaulthost.docker.internalis ASCII, no structural chars, not an IP, no colon → stored verbatim.- Structural char set is complete for authority boundaries.
_STRUCTURAL_CHARScovers every RFC 3986 gen-delim that moves a boundary (/ ? # @ [ ]), plus\(WHATWG-normalized to/),%(RFC 6874 zone IDs and percent-encoded delimiters like%2F/%40that_normalize_pctwould later decode), and theord < 0x21 or == 0x7Fcontrol/whitespace sweep. The absent sub-delims (! $ & ' ( ) * + , ; =) are legalreg-namechars that neitherurlsplitnor_canon_authoritysplit on — not a gap.security-reviewer: clean, no netloc-boundary bypass survives. - Bracket-unwrap can't smuggle.
inner = value[1:-1]strips one outer pair only, andinneris still scanned for[/], so[[::1]],[::1]evil],[a]b[c]all reject. :handling is correct: excluded from the structural set so IPv6 literals survive, then re-caught after a failedip_address()parse as an embedded port — whichapply_hooks' port guard would otherwise flag.- Signed-authority spelling is RFC-correct. Bracketing bare IPv6 and folding to compressed lowercase matches RFC 3986 §3.2.2 / RFC 5952 §4, and
canonicalize_authority("https://[2001:db8::1]:9000/hook")returns[2001:db8::1]:9000— stable and verifiable by a conformant receiver running the same canonicalization.ad-tech-protocol-expert: sound. - Correctly does not route IP literals through
_idna_canonicalize.canonicalize_host(which returns IPv6 unbracketed, being a comparison helper) —canonicalize_hostis only called on the non-ASCII IDN branch, where the input is guaranteed non-IP. Clean separation. - Docker-legal underscore names (
my_service,host_gateway,_dns-sd._udp.local) accepted structurally rather than rejected by IDNA — the right decision, and pinned bytest_rewrite_to_accepts_docker_legal_service_namesso a future IDNA "tidy-up" fails loudly. - Public surface:
DockerLocalhostRewriteis a public export; the change rejects previously-malformed values and normalizes valid ones. No signature change, no removed export, no required↔optional flip.fix(webhooks):is the correct semver signal. - Test plan:
make ci-localreported 5911 passed; 9 of the 25 signing conformance tests fail against the pre-fix module (verified by the author in a throwaway worktree, not by reasoning). No unchecked manual boxes.
Follow-ups (non-blocking — file as issues)
rewrite_to=\"[]\"stores an empty host.[]is non-empty so it clears theif not valueguard, unwraps toinner=\"\", and — empty structural scan,ip_address(\"\")raises, no colon,\"\".isascii()isTrue— lands stored as\"\".rewrite_urlthen emitshttps://:9000/hook, andapply_hooks' scheme/port re-check passes because neither changed, so a malformed empty authority reaches SSRF/signing. Self-inflicted operator config and SSRF stays authoritative, so it's Low — but a one-lineif not inner:re-check right after the unwrap closes it. (Flagged independently bysecurity-reviewer.)- IPv4-mapped spelling is Python-canonical, not RFC 5952 §5.
::ffff:127.0.0.1folds to[::ffff:7f00:1](pure hextet) rather than the SHOULD-form mixed[::ffff:127.0.0.1]. Both are valid RFC 3986IPv6address, and it doesn't break AdCP signing becausecanonicalize_authoritynever re-folds IPs — the only theoretical break is a receiver that independently RFC-5952-normalizes before verifying. Worth a one-line docstring note that the spelling is Python-canonical.
Minor nits (non-blocking)
- PR body and commit name the wrong method. The "Placement" section says validation lives in
validate_for_sender"rather than__post_init__" — but the code puts it in__post_init__. The body's reasoning ("covers directapply_hooksuse and hooks not attached to a sender — strictly broader") describes__post_init__exactly, so the code is the better choice and the prose is the thing that's wrong. An interesting inversion — the shipped code is broader than its own changelog claims. Fix the body/commit to say__post_init__so a future auditor reasons about the right entry point. - Comment implies the IP-parse rejects zone IDs; the structural filter does. The zone-ID comment reads as though
ipaddress.ip_addresscatchesfe80::1%eth0, but%is in_STRUCTURAL_CHARSand rejects it several lines earlier —ip_address(which would accept the scoped form) is never reached. Cosmetic. - Unconditional signing import.
from adcp.signing._idna_canonicalize import canonicalize_hostruns at the top of__post_init__even on the default/ASCII and pure-IP paths that never call it; moving it into the non-ASCII branch would spare them the coupling. No circular-import risk. Low priority.
Approving on the strength of the structural-over-syntactic invariant plus the signing-canonicalization round-trip test. Follow-ups noted.
bokelley
left a comment
There was a problem hiding this comment.
The construction guard still accepts inputs that normalize to an empty host. Reproduced on current main + this PR:\n\n- DockerLocalhostRewrite(rewrite_to="[]").rewrite_to == ""\n- rewrite_url("https://localhost:9000/hook") returns "https://:9000/hook"\n- DockerLocalhostRewrite(rewrite_to=".").rewrite_to == ""\n\nThe first passes because unwrapping happens after the initial non-empty check; the second passes because the ASCII branch strips all trailing dots without rechecking. Please reject an empty inner value and an empty post-normalization value, with regression coverage for both. The focused suite otherwise passes (29 tests).
There was a problem hiding this comment.
Clean fix. Right shape: rewrite_to is the one non-signing layer that synthesizes the netloc that later becomes the signed @authority, so structural validation at construction is exactly where a mis-split turns into a wrong signature instead of a failed request. Fail-closed beats fail-open here.
Things I checked
- The structural-before-parse ordering is load-bearing.
_STRUCTURAL_CHARS(webhook_transport_hooks.py:46) scansinnerbeforeipaddress.ip_address, so%catches RFC 6874 zone IDs (fe80::1%eth0) beforeipaddresswould accept the scoped form. Every URL-boundary char is covered —/@?#terminators,@userinfo,\(WHATWG backslash normalization),[], plus control chars viaord < 0x21/0x7F.:is correctly excluded and handled by the IP-literal parse / non-IP port check. - The two empty-host routes the follow-up commit closes.
"[]"(webhook_transport_hooks.py:113, non-empty until brackets come off) and"."/".."(single-dot strip, notrstrip('.'), so".."→"."→ empty label) both used to assemble tohttps://:9000/hook. The "no empty label" phrasing atwebhook_transport_hooks.py:186also covers the interior"a..b"case. Correct. - Bare IPv6 bracketing survives the full path.
[{ip}]folding (webhook_transport_hooks.py:145) meansapply_hooks'new.portre-parse at:146doesn't raise, andcanonicalize_authorityyields[2001:db8::1]:9000. IPv4-mapped v6 folds to canonical[::ffff:7f00:1].object.__setattr__is the correct frozen-dataclass idiom. - No canonicalization mismatch.
security-reviewerconfirmed inwebhook_sender._send(webhook_sender.py:974-1023) the singleeffective_urlstring is what gets SSRF-validated, signed, and POSTed — sign-target and connect-target derive from the identical string, so there's no sign-as-one/connect-as-another gap. This guard is signed-URL integrity, not the SSRF boundary; SSRF still runs authoritatively after the hook. - Not a breaking change.
DockerLocalhostRewriteis public, but everything the guard now rejects at construction was already broken — rejected downstream by the pinned transport or silently mis-signed.fix(webhooks):is the correct semver signal; no!needed. Verified underscore Docker names (my_service,host_gateway,_dns-sd._udp.local) still accepted — the structural-not-IDNA decision is the right call for the class that exists to serve Docker. - No import cycle:
adcp.signingnever referenceswebhook_transport_hooks, so the deferredcanonicalize_hostimport can't loop.
Follow-ups (non-blocking — file as issues)
- UTS46 silently deletes mapped/ignored codepoints (
webhook_transport_hooks.py:200). A soft hyphen (U+00AD) in a non-ASCIIrewrite_tois stripped, so"host.example"canonicalizes to"host.example"— self-consistent (sign == connect), operator config only, not exploitable. But the docstring frames this branch as fail-loud, and some inputs mutate silently. Worth a log line when the canonical form differs beyond case/bracket/trailing-dot. - Structural-only ASCII acceptance permits reg-name sub-delims (
webhook_transport_hooks.py:180-193).foo=barpasses structurally but httpx may reject at send time — a per-delivery failure rather than a wiring-time one. Explicit documented tradeoff to keep underscores working; noting it, not blocking it.
Minor nits (non-blocking)
- PR-body "Placement" paragraph is inverted. It says validation lives in
validate_for_sender"rather than__post_init__" — the code does the opposite (__post_init__), and__post_init__is the only placement that actually covers directapply_hooksuse and unattached hooks, sincevalidate_for_senderfires only when a hook is wired to a sender. The code is correct and strictly broader; the prose argues for the code's opposite. Fix the description so a future reader trusting it doesn't conclude the reverse of what ships. - Deferred import is unconditional (
webhook_transport_hooks.py:97-101) butcanonicalize_hostis used only on the non-ASCII branch. Cosmetic — after first import it's asys.moduleshit — but it could move inside that branch so the common IP/ASCII paths never touch the signing package. - Docstring at
webhook_transport_hooks.py:77-79still describes only thevalidate_for_sender/allow_private_destinationscheck; the newrewrite_tocanonicalization contract is documented in the class docstring above, so this is fine, but the two paragraphs read as ifvalidate_for_senderis the only construction-time check.
25 test cases, 9 of which fail against the pre-fix module — the underscore-name pin (test_rewrite_to_accepts_docker_legal_service_names) is a good regression fence against a future IDNA "tidy-up."
Approving on the strength of the structural-before-parse ordering plus the sign/connect same-string guarantee.
|
Fixed in Each reached the empty host past a different check:
So the rule is now stated as "no empty label" rather than "not empty", which also covers an interior empty label ( You framed it as an empty host; the sharper version is that the assembled URL was
|
…te rewrite_to `rewrite_to` was interpolated straight into a netloc with no validation. A bare IPv6 value produced an unbracketed authority -- the one shape RFC 3986 makes ambiguous with a port -- so `rewrite_to="::1"` against a URL with a port yielded `https://::1:9000/hook`, which `_canon_authority` mis-splits at the last colon. This is the only place a non-signing layer synthesizes the netloc that later becomes the signed `@authority`. Bare IPv6 literals are now bracketed at construction (`"::1"` -> `"[::1]"`) and folded to canonical compressed form, so the assembled authority is unambiguous and the signed value is stable. The validation is deliberately STRUCTURAL, not a hostname-syntax check. It rejects only what can move the boundary between authority, userinfo, port, path, query or fragment -- path injection, userinfo injection, embedded ports, raw whitespace and control characters, and RFC 6874 IPv6 zone IDs (`%` cannot appear unencoded in a URI authority). It deliberately does NOT enforce hostname syntax. Docker Compose service names legally contain underscores (`my_service`, `host_gateway`) which RFC 952/1123 and IDNA both reject, and Docker's embedded DNS resolves them. Routing `rewrite_to` through the IDNA canonicalizer would have rejected those at construction -- turning a working deployment into a startup crash on upgrade, in the class that exists specifically to serve Docker. Whether a name resolves is the resolver's business; whether it restructures the signed URL is ours. Non-ASCII names are still IDNA-encoded to A-labels, since those cannot go on the wire as-is. Validation lives in `validate_for_sender` rather than `__post_init__` so it also covers direct `apply_hooks` use and hooks not attached to a sender. Fixes adcontextprotocol#991.
Review follow-up. The guard rejected a structurally unsafe value but still
let three shapes through to an empty host, which then assembled to
`https://:9000/hook` -- an authority with a port and no host, exactly the
shape @target-uri canonicalization rejects, produced by the hook meant to
keep the authority well-formed.
Each reached the empty host past a different check:
- `"[]"` is non-empty until the brackets come off.
- `"."` is non-empty until the trailing root dot comes off.
- `".."` survived a single-dot strip as `"."`, because the strip used
rstrip('.') and ate every dot rather than the one root dot
canonicalize_host removes.
Rejection is now stated as 'no empty label', which also covers an interior
empty label (`"a..b"`).
Refs adcontextprotocol#991.
c81fee7 to
9de2b4f
Compare
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
bokelley
left a comment
There was a problem hiding this comment.
Verified the follow-up rejects bracket-empty, root-dot-empty, and interior-empty-label rewrite targets while retaining the intended Docker-compatible names. My previous request is fully addressed.
Fixes #991.
Independent of the signing stack — branched off
main, touches no file #980/#985/#987/#993 touch.What was wrong
DockerLocalhostRewrite.rewrite_tois interpolated straight into a netloc with no validation. A bare IPv6 value produces an unbracketed authority — the one shape RFC 3986 makes ambiguous with a port:This is the only place a non-signing layer synthesizes the netloc that later becomes the signed
@authority, so a mis-split here is a wrong signature rather than a failed request.The validation is structural, not syntactic — and that distinction is the point
It rejects only what can move the boundary between authority, userinfo, port, path, query or fragment: path injection, userinfo injection, embedded ports, raw whitespace, control characters, and RFC 6874 zone IDs.
It deliberately does not enforce hostname syntax, and I want to flag that as a decision rather than an omission.
The obvious implementation is to route
rewrite_tothrough_idna_canonicalize.canonicalize_host— the SDK is IDNA-strict nearly everywhere else. That rejectsmy_service,host_gateway,_dns-sd._udp.local: legal Docker Compose service names, resolvable by Docker's embedded DNS, illegal under RFC 952/1123 and IDNA.DockerLocalhostRewriteexists specifically to serve Docker deployments. Rejecting Docker-legal names at construction would turn a working deployment into a startup crash on upgrade — and it would do so in the operator-supplied-client path (webhook_sender.py:1013-1023), which skips the pinned transport entirely, so those configurations genuinely work today end-to-end. Verified:canonicalize_authority("https://my_service:9000/hook")returnsmy_service:9000cleanly onmain.Whether a name resolves is the resolver's business. Whether it restructures the signed URL is ours. Non-ASCII names are still IDNA-encoded to A-labels, since those cannot go on the wire as-is.
Measured behaviour:
Placement
validate_for_senderrather than__post_init__, so it also covers directapply_hooksuse and hooks not attached to a sender — strictly broader than the issue's suggested direction.Adopter-visible change
Values that cannot be represented in a netloc now raise at construction instead of producing a malformed authority at first delivery. Everything the guard rejects was already broken — either rejected downstream by the pinned transport, or silently mis-signed. Nothing that works today stops working; that is the whole reason the guard is structural rather than IDNA-based.
Test plan
make ci-local: 5911 passed, 0 failed, 41 skipped, 1 xfailed.25 tests in
tests/conformance/signing/test_webhook_transport_hooks.py. 9 of them fail against the pre-fix module — verified by checkingmain's version of the file over the branch's in a throwaway worktree and re-running, not by reasoning.One test exists purely to stop a future "tidy-up" from reintroducing the problem:
test_rewrite_to_accepts_docker_legal_service_namespins the underscore names, so swapping the structural check for an IDNA one fails loudly instead of quietly breaking Docker users.